This won't really matter, but the unnecessary "packed" shows that I
didn't really understand the difference between packed and unpacked
structs in SystemVerilog when I wrote the code in 2022. Or indeed
yesterday morning...

Signed-off-by: Rupert Swarbrick <[email protected]>

For example, consider flash_mp_region_cfg_t. Structs of this type are
randomised in several places (see flash_ctrl_error_mp_vseq, for
example). This isn't going to do what we need if the struct is packed.
For example (without further constraints), each of those seven mubi4_t
elements is going to be have an invalid encoding with probability
14/16.

This might be worked around. Indeed, I see that
flash_ctrl_error_mp_vseq has explicit constraints for the regions,
that will force the fields to take named enum values. But it's
probably more sensible to default to more guessable behaviour.

As well as flash_ctrl_error_mp_vseq, this commit makes the same change
to:

  - flash_bank_mp_info_page_cfg_t (another 7 mubi values, with the
    same analysis).

  - flash_op_t (contains several enums and gets confusingly randomised
    in several places, which is why I noticed this in the first place)

  - rd_cache_t (contains an enum variable)

It turns out that removing "packed" isn't quite enough. The biggest
reason is that there were quite a few "solve before" lines that aren't
actually allowed if you have something more interesting than a bit
vector.

Changes needed:

  - Make the fields of flash_op_t themselves be rand. This is needed
    because calling my_class.randomise(some_class_variable) doesn't
    work if some_class_variable is an unpacked struct whose fields
    aren't explicitly marked rand.

  - This is also needed in flash_mp_region_cfg_t and
    flash_bank_mp_info_page_cfg_t. For some reason, *that* isn't
    visible at compile time, but you end up with a runtime error where
    something is randomised with X values (not allowed by the spec!)
    and the root cause is that we're not actually randomising e.g. a
    region config struct, so it gets its initial value (of X).

  - Rephrase lots of rules of the form "solve xyz before
    flash_op" to things like "solve xyz before flash_op.addr". This is
    because the SystemVerilog spec doesn't allow anything except an
    integer in as a thing to be solved before/after. I think the DV
    engineer just has to pretend to be a compiler. Sigh...

  - Remove some ordering constraints of the form "solve en_mp_regions
    before mp_regions". This is definitely not allowed (because
    mp_regions is an array of unpacked structs, so not a bit vector).
    I'm not completely convinced we care but, if we do, my proposal
    would be to randomise the items of the vector explicitly as we get
    to them ("late randomisation"). The code ends up simpler, I think.

  - Fix flat-out bogus syntax in flash_ctrl_invalid_op_vseq.sv.
    Antonio and I have fixed the same error in quite a few places in
    the past. In this case, it came from commit 9b01461 in 2022.

Signed-off-by: Rupert Swarbrick <[email protected]>

There is no functional change, but dropping the ASSERT macro makes
things a little easier to read.

The hardware reset assertions should now be a little more
efficient. I don't think we really need to bother asserting that the
signals don't magically start changing somewhere in the middle of a
reset(!), so I've restructured them slightly to check the first clock
after the start of a reset.

The first syntax I used for this (which is equivalent to the previous
version, but without noise from the macro) uses two "clocks" and is of
the form

  @(negedge rst_aon_ni) 1 |=> @(posedge clk_aon_i) XYZ

Here, the |=> waits for the event on the right hand side, so it means
"On the first clock edge after reset is asserted, XYZ". Very nice!

Unfortunately, this isn't actually supported by Verible's
SystemVerilog front-end. Grr! I was very annoyed at first, but
realised that there's actually an even cleaner way to do it. Because
this is a bound-in interface, we can easily define a helper signal.

This commit defines a "start_of_reset" flag, which set when reset is
asserted and then cleared on the first clock. Now, the properties can
be very simple, looking like:

  start_of_reset -> XYZ

(checked on the positive edge of clk_aon_i because of the default
clocking statement).

That's even shorter! Yippee!

When I initially wrote this, I worried about the scheduling because we
clear start_of_reset on the clock edge and look at the value at the
same time. Fortunately, this is ok: the value tested in the expression
is its value from the preponed region (see IEEE 1800, 16.5.1), so
we'll get the value that hasn't yet been cleared.

Signed-off-by: Rupert Swarbrick <[email protected]>